Skip to content
This repository has been archived by the owner on Nov 9, 2023. It is now read-only.

Q3 2022 repository standardization #25

Merged
merged 9 commits into from
Jul 26, 2022
Merged

Q3 2022 repository standardization #25

merged 9 commits into from
Jul 26, 2022

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Jul 17, 2022 •
edited
Loading

Standardizes this repository per the module template as of Q3 2022. All changes are copied over directly from the template, except for the tests, which have been rewritten. A couple of notes:

  • I didn't bother rewriting the tests for JSON-RPC notifications, as that functionality will be implemented in a completely different manner in Add DuplexJsonRpcEngine stream #24 has been merged.
  • CI has been switched from CircleCI to GitHub Actions.

@rekmarks rekmarks requested a review from a team as a code owner July 17, 2022 02:14
@rekmarks rekmarks marked this pull request as draft July 17, 2022 02:14
@rekmarks rekmarks force-pushed the repository-standardization branch from d728c37 to 0a26223 Compare July 17, 2022 02:19
@rekmarks rekmarks marked this pull request as ready for review July 17, 2022 02:28
@rekmarks
Copy link
Member Author

Status checks are failing because of the switch to GitHub Actions. I'll fix this once I have an approval.

mcmire
mcmire reviewed Jul 18, 2022
View reviewed changes
Copy link

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware we had this package 😅 One small thing but otherwise looks good to me.

tsconfig.json Outdated
"./node_modules/@types",
],
"target": "es2017",
"typeRoots": ["./node_modules/@types"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need this setting, as this is the default value: https://www.typescriptlang.org/tsconfig#typeRoots

Copy link

@mcmire mcmire Jul 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, take that back, this tells TypeScript to use @types/* packages that are located immediately within node_modules as opposed to anywhere else, but that said, we don't usually have to mess with this setting, so still I'm not sure we need to set it.

Copy link
Member Author

@rekmarks rekmarks Jul 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. I just preserved it because it was already there. Removed in 042dd7a.

mcmire
mcmire approved these changes Jul 20, 2022
View reviewed changes
Copy link

@mcmire mcmire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Note that there are two more PRs I merged into the module template just now — one to upgrade to Yarn v3, another to tweak a TypeScript setting. Up to you on whether you want to include those now or later. As for this PR though, looks good.

@rekmarks rekmarks merged commit 8c64407 into main Jul 26, 2022
@rekmarks rekmarks deleted the repository-standardization branch July 26, 2022 23:22
@MajorLift MajorLift mentioned this pull request Nov 10, 2023
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mcmire mcmire mcmire approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rekmarks @mcmire